feat(create-pr): add reuse_branch to keep one self-updating pull request - #80
Conversation
5b9249d to
4efc5fc
Compare
4efc5fc to
40e8fec
Compare
40e8fec to
95ed6c8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated git helpers still interpolate unquoted branch names into shell commands (execSync), which is a concrete command-injection risk when inputs are attacker-controlled.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a reuse_branch mode to the actions/create-pr action to keep a single self-updating PR (instead of opening a new PR on each run), and wires this behavior into the reusable generate-changelog workflow (including concurrency protection) to prevent changelog PR accumulation and long-lived conflicts.
Changes:
- Add
reuse_branchinput plus “updated vs created” outputs, and implement PR find/update/close behavior inactions/create-pr. - Extend git helpers to support branch reset (
git checkout -B) and force push (git push --force) when reusing a branch. - Enable
reuse_branchby default for thegenerate-changelogreusable workflow and add a concurrency group to prevent branch force-push races; update READMEs accordingly.
File summaries
| File | Description |
|---|---|
| workflows/generate-changelog/README.md | Document workflow behavior/inputs including reuse_branch and concurrency note. |
| .github/workflows/generate-changelog.yaml | Add reuse_branch input defaulting to true and enforce concurrency to prevent races. |
| actions/create-pr/action.yaml | Declare new reuse_branch input and new pull_request_updated output. |
| actions/create-pr/src/main.ts | Implement branch reuse logic: stable head name, close obsolete PR on no-op, detect existing PR and mark as updated. |
| actions/create-pr/src/create-pull-request.ts | Add helpers to find an open PR by head/base and close it when obsolete. |
| actions/create-pr/src/create-commit.ts | Reset and force-push when reusing a long-lived branch. |
| actions/create-pr/README.md | Document new input/output semantics. |
| lib/action-utils/src/git.ts | Add options to checkout/push helpers to support reset and force push. |
| actions/create-pr/dist/index.js | Rebuilt bundle reflecting new create-pr behavior. |
| actions/assemble-docs/dist/index.js | Rebuilt bundle reflecting updated git helpers. |
| actions/submit-docs/dist/index.js | Rebuilt bundle reflecting updated git helpers. |
| actions/extract-version/dist/index.js | Rebuilt bundle reflecting updated git helpers. |
Review details
- Files reviewed: 8/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
reuse_branch introduces a force-push path without guarding against branch_name === base_branch_name, which could reset and force-push the base branch if misconfigured.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 9/13 changed files
- Comments generated: 1
- Review effort level: Lite
|
Valid and fixed in 78061bb. With the random suffix,
if (head === base) {
throw new Error(
`branch_name resolves to the base branch '${base}'. Set branch_name to a different branch.`,
);
}The guard sits at main.ts:36, ahead of |
There was a problem hiding this comment.
🔵 Needs a closer look
gitPushBranch still allows git option injection via branch names beginning with -- unless an end-of-options marker is added before the refspec.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
lib/action-utils/src/git.ts:44
gitPushBranchpasses the branch name as the final argument togit pushwithout an explicit--end-of-options marker. If the input branch name begins with-/--(e.g.--all,--mirror), Git can parse it as an option rather than a refspec, potentially pushing unintended refs or otherwise changing push behavior. Add--before the branch/refspec to prevent option injection and to support such branch names safely.
- Files reviewed: 9/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Valid, fixed in abdd46c. Confirmed the behaviour rather than assuming it: So Two things testing changed about the fix. It is limited to the push, because
On severity, to be accurate rather than alarming: this was not reachable through |
There was a problem hiding this comment.
🟡 Changes recommended
gitCheckoutBranch still lacks a -- end-of-options separator, allowing branch names beginning with - to be parsed as git options (option-injection risk) despite the other hardening work.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
lib/action-utils/src/git.ts:46
- This comment is inaccurate:
git checkout -b/-Bdoes have an equivalent--end-of-options separator (and should use it) to prevent branch names starting with-from being parsed as options. Leaving this as-is is misleading for future maintenance.
// `--` keeps a branch name that begins with a dash from being read as an
// option: `git push origin --mirror` pushes every ref rather than a branch.
// `git checkout -B` needs no equivalent, and rejects such a name itself.
- Files reviewed: 9/13 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new concurrency grouping does not fully prevent force-push races when the reusable workflow is invoked from multiple refs using the same reused branch_name.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/workflows/generate-changelog.yaml:87
- The concurrency group is keyed on
github.ref(the triggering ref), but the race you’re preventing is on the reused head branch (inputs.branch_name). If this reusable workflow is invoked for multiple refs (e.g. main + a release branch) while keeping the samebranch_name, runs can still overlap and force-push the same branch concurrently.
workflows/generate-changelog/README.md:11 - This note explains that runs are serialized to avoid races when
reuse_branchreuses a single branch, but it doesn’t mention that the reusable workflow itself now setsconcurrency. Callers reading this README may still add their own concurrency block unnecessarily (and potentially with a different grouping).
- Files reviewed: 9/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Both suppressed comments were worth it, and the first one is a real bug in my own change. Fixed in 6f12813. The concurrency key was wrong. The group was - group: ${{ github.workflow }}-${{ github.ref }}
+ group: generate-changelog-${{ inputs.branch_name }}
Checked that The README point was fair as well. It described the effect without saying the workflow sets Worth recording that this is the fourth pass on this PR and the first three each found something real: shell injection, the base-branch force push, then option injection on |
There was a problem hiding this comment.
🟡 Changes recommended
The updated documentation for the reusable generate-changelog workflow and create-pr action contains incorrect defaults/usage that can cause copy-paste or “rely on defaults” misconfiguration.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
actions/create-pr/README.md:27
- The README lists the default token as
${{ GITHUB_TOKEN }}, but that expression is not valid in workflow syntax (it should be${{ github.token }}or${{ secrets.GITHUB_TOKEN }}). Aligning this withaction.yamlprevents copy/paste failures.
workflows/generate-changelog/README.md:21
- The documented defaults for
pull_request_title/pull_request_bodydon’t match the workflow’s actual defaults in.github/workflows/generate-changelog.yaml(which are changelog-specific). Keeping these aligned avoids surprising PR titles/bodies when callers rely on defaults.
| `pull_request_title` | The title of the pull request. | `'chore: automated by GitHub actions'` |
| `pull_request_body` | The body of the pull request. | `'This pull request was automatically created by a GitHub Action.'` |
workflows/generate-changelog/README.md:24
- The documented default for
commit_messagedoes not match the workflow’s actual default in.github/workflows/generate-changelog.yaml(it defaults tochore: generate changelog).
| `commit_message` | The message of the commit. | `'chore: automated by GitHub actions'` |
- Files reviewed: 9/13 changed files
- Comments generated: 1
- Review effort level: Lite
Reusing a branch resets and force pushes it, so a branch_name equal to base_branch_name would have force pushed the base branch. The random suffix made that unreachable before, and reuse removes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Passing arguments directly stops a shell reading them, but git still parses a leading dash as an option: `git push origin --mirror` pushes every ref rather than a branch of that name. git checkout -B needs no marker and rejects such a name itself, so the change is limited to the push. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The branch is a trailing positional on push and the value of an option on checkout, which is why only one of them needs the marker. Stating the mechanism rather than the conclusion, since the previous wording read as an unexplained asymmetry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The group used the triggering ref, but the contended resource is the reused head branch. Two refs invoking the workflow with the same branch_name landed in separate groups and could still force push it concurrently, which is the race the group was added to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reusable workflow's input table was copied from the create-pr action
and never updated, so branch_name, the pull request title and body, and
the commit message all documented the action's defaults rather than the
workflow's. A caller relying on them would have got a different branch
and different commit text than described.
The action's token default was also written as ${{ GITHUB_TOKEN }},
which is not valid workflow syntax.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The input described updating a pull request without mentioning that the branch is reset and force pushed first, which is the operationally significant part and the reason it suits only a branch owned by automation. Stated in the input description and in both READMEs, the reusable workflow included since it enables this by default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
environment is required but was absent from both the input table and the example, so the example failed when copied. file_name, owner and repository were missing as well. The example also carried a caller concurrency block, which the workflow now provides itself keyed on the branch it pushes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4378659 to
c3afc0e
Compare
#80 changed actions/create-pr, so the workflows still referenced the copy from before it and reuse_branch would have been passed to an action that does not declare it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Picks up dfinity/ci-tools#94. `bdfe993..a98f5d0` is only that commit. Since `reuse_branch` (dfinity/ci-tools#80), the changelog pull request is force pushed on every push to `main`, and those pushes came from the default token as `github-actions[bot]`. Its workflows then waited for approval, and required `pull_request_target` workflows such as the External PR Ruleset never ran, which blocks the pull request even when approved (e.g. dfinity/pic-js#290). With #94, the branch is pushed with the app token instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Picks up dfinity/ci-tools#94. `bdfe993..a98f5d0` is only that commit. Since `reuse_branch` (dfinity/ci-tools#80), the changelog pull request is force pushed on every push to `main`, and those pushes came from the default token as `github-actions[bot]`. Its workflows then waited for approval, and required `pull_request_target` workflows such as the External PR Ruleset never ran, which blocks the pull request even when approved (e.g. dfinity/pic-js#290). With #94, the branch is pushed with the app token instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Picks up dfinity/ci-tools#94. `bdfe993..a98f5d0` is only that commit. Since `reuse_branch` (dfinity/ci-tools#80), the changelog pull request is force pushed on every push to `main`, and those pushes came from the default token as `github-actions[bot]`. Its workflows then waited for approval, and required `pull_request_target` workflows such as the External PR Ruleset never ran, which blocks the pull request even when approved (e.g. dfinity/pic-js#290). With #94, the branch is pushed with the app token instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Picks up dfinity/ci-tools#94. `bdfe993..a98f5d0` is only that commit. Since `reuse_branch` (dfinity/ci-tools#80), the changelog pull request is force pushed on every push to `main`, and those pushes came from the default token as `github-actions[bot]`. Its workflows then waited for approval, and required `pull_request_target` workflows such as the External PR Ruleset never ran, which blocks the pull request even when approved (e.g. dfinity/pic-js#290). With #94, the branch is pushed with the app token instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Picks up dfinity/ci-tools#94. `bdfe993..a98f5d0` is only that commit. Since `reuse_branch` (dfinity/ci-tools#80), the changelog pull request is force pushed on every push to `main`, and those pushes came from the default token as `github-actions[bot]`. Its workflows then waited for approval, and required `pull_request_target` workflows such as the External PR Ruleset never ran, which blocks the pull request even when approved (e.g. dfinity/pic-js#290). With #94, the branch is pushed with the app token instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Closes #77.
create-pradds a random suffix tobranch_nameon every run, so each run opens a new branch and a new pull request. While an earlier one is unmerged the next run still sees a diff and opens another, and after a release they all go permanently conflicting.icp-js-coreaccumulated 8 changelog pull requests for 2 distinct states;pic-jshas 6 open, 3 conflicting.Behaviour changes
reuse_branchinput onactions/create-pr. When enabled,branch_nameis used as-is, the branch is reset and force pushed, the pull request already open for it is updated rather than duplicated, and it is closed once there is nothing left to propose. Newpull_request_updatedoutput.generate-changelogenables it by default, so the five consuming repos move from one pull request per run to a single self-updating one.create-prrefuses abranch_nameequal tobase_branch_name, which would otherwise have force pushed the base branch.git pushends option parsing before the branch name. A branch name, commit message or author value can no longer be read as shell syntax, nor a branch name as a git option.For the reviewer
--forcerather than--force-with-lease: the branch is reset from the base every run, so a lease check would reject every push. Enablingreuse_branchasserts the branch is automation-owned, which the input description now says.Not exercised live. The find, update and close paths need a real push to
mainin a consuming repo.Two other callers have the same bug but keep
false.npm-auditin icp-js-core is safe to flip.pull-project-docsin icp-js-sdk-docs is not: six projects share one branch name and each run resets tomainafter emptying only its own subdirectory, so reuse would discard another project's unmerged update. It needs per-project branch names first.🤖 Generated with Claude Code